This is most excellent. Thank you.
There are integration tests: https://github.com/aws/aws-encryption-sdk-javascript/blob/master/modules/integration-node/src/decrypt_materials_manager_node.ts#L86

This will also need to be updated.
I feel pretty confident that the OAEP hash is the same as the MGF1 hash,
but these integration tests will verify.
If you want to take a crack at that as well you are welcome,
but I can also push changes to this branch later this week.

👍 thanks!

I had a go at adding support in the integration tests. I haven't been able to run them myself because I don't fully understand the setup (they seem to want to connect to KMS, which I'm not going to let them do). Hopefully CI should run them for me, but if it's using a node version older than 12.9 they'll probably fail, at which point I might need some help - we'll see how we go :)

I guess the failing build is due to < 12.9 node version in the integration tests, but I can't see the output. Over to you @seebees.

You are right. The issues is v10 vs v12. We are going to need 3 things.

1. A precondition for: "The AWS ESDK only supports specific hash values for OAEP padding." for the RawRsaKeyringNode constructor.

OpenSSL will take, and Node.js will pass, any hash as a valid hash. But the ESDK only supports SHA-1 and SHA-2 :) . Also it will pass through the hash even if you select RSA_PKCS1_PADDING.

2. A test to cover the precondition. Everything that has such a comment requires a test.

3. The rsaPadding function in the integration module will need to sniff Node.js to see if it supports oaepHash or not.

I suppose the best way is just to try it in the initialization. Ugly but practical.
The integration tests should be able to run on different versions of Node.js and reasonably report what vectors are supported :)

To run the integration tests, all that is needed is AWS credentials.
The KMS keys we use for these tests are public and must never be use for anything other than our testing.

npm run node-integration will run them if you want to try it out locally.
Otherwise I'll button this up and push some commits late tomorrow.

Thanks again!

Thanks - sounds good.

On the precondition, I can't think of a nice way of feature detecting support for the oaepHash parameter. I think the options are:

• Decrypt a known payload with a known key in the constructor using sha256 OAEP - if it doesn't work then we only support sha1 - this feels like a lot of work to be doing in the constructor though
• publicEncrypt.toString().includes('oaepHash') - obviously super gross, but should work
• Don't do feature detection, and just do version detection like:

function supportsOaepHash() {
  const [major, minor] = process.version.replace('v', '').split('.').map(x => parseInt(x, 10))
  return major > 12 || major === 12 && minor >= 9
}

I guess that by "I suppose the best way is just to try it in the initialization" you mean you'd prefer the first option?

I did some work on this,
I'll push a commit today and you can take a look :)

I do think that actually testing it is, unfortunately, the way to go here.
Because if you accept the oaepHash parameter
on a version of Node.js
that does not support the option,
it will silently "work".
Now we have something encrypted
that does not match the security properties that were requested.
There is a part of me that enjoys stringfying the function,
but that is a little too clever.
And finally, parsing the version will not catch back ports.

@richardTowers Turns out I don't have access to push to your fork :(
If you want to give me access, I can push these changes and we can talk about them.
You can take a look at there here

Ah yes - I've just invited you to collaborate on the fork, so hopefully you should be able to push now.

Your code looks good to me - it's true that the feature detection is a bit confusingly backwards, but it seems robust. Shame there's no runtime types or reflection in JS, but you work with what you've got I suppose.

Pushed.
Since the older version of the function will ignore the parameter,
I really really don't want to be in a situation where a customer expects something to be true and it was not :)

Agree completely, yeah. I'm happy with the commit you pushed, so as far as I'm concerned this PR is good to go.

That said, I haven't run the integration tests against node v12, and I'm guessing CI hasn't either. It looks like you've already got an issue to test against a matrix of node versions (#206), but perhaps a manual run would be enough for this PR.

Also note, if this lands, AWS should update this documentation to call out support in newer node versions:

https://docs.aws.amazon.com/encryption-sdk/latest/developer-guide/javascript.html#javascript-language-compatibility (edit: I had the wrong link)

Thanks. I have run this agains v12 locally.
I will again now that we are in agreement on the final code.
Thanks for the doc note as well.
Coordinating that should not slow down the merge,
but might slow down the release.